chore: Update to use current upstream modules #422
+133
−120
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Andrews-repo
changed the title
Andrews-repo
changed the title
Andrews-repo
changed the title
|
@bryantbiggs I know you are a very busy man, and I apologize for bothering you, but is there anything i can do to assist with getting this reviewed and merged in? I have colleagues at other orgs hoping to use this update to the module |
|
I'll take a look this evening - apologies for the delay |
|
No worries, I know you can be slammed with all the modules, and always get a laugh when you remind people all this is offered for free, so I wasnt going to rush you or anything. I appreciate your time |
bryantbiggs
reviewed
Aug 5, 2025
| @@ -125,7 +125,7 @@ resource "random_password" "webhook_secret" { | |||
|
|
|||
| module "secrets_manager" { | |||
| source = "terraform-aws-modules/secrets-manager/aws" | |||
| version = "~> 1.0" | |||
| version = "1.3.1" | |||
There was a problem hiding this comment.
in examples we don't pin to a specific version, so lets revert changes like this
bryantbiggs
requested changes
Aug 5, 2025
| @@ -1,10 +1,10 @@ | |||
| terraform { | |||
| required_version = ">= 1.0" | |||
| required_version = ">= 1.10" | |||
There was a problem hiding this comment.
we are only raising to 1.5.7 at this time unless there is a strong reason for going higher - this should be carried throughout
Suggested change
| required_version = ">= 1.10" | |
| required_version = ">= 1.5.7" |
|
|
||
| tags = var.tags | ||
| } | ||
|
|
||
| module "ecs_service" { | ||
| source = "terraform-aws-modules/ecs/aws//modules/service" | ||
| version = "5.11.0" | ||
| version = "6.1.1" | ||
|
|
||
| create = var.create | ||
|
|
||
| # Service | ||
| ignore_task_definition_changes = try(var.service.ignore_task_definition_changes, false) |
There was a problem hiding this comment.
with the variable optional attributes, a lot of these try(() blocks should be removed
main.tf
Outdated
|
|
||
| create = var.create | ||
|
|
||
| # Service | ||
| ignore_task_definition_changes = try(var.service.ignore_task_definition_changes, false) | ||
| alarms = try(var.service.alarms, {}) | ||
| alarms = try(var.service.alarms, { alarm_names = [] }) |
| @@ -278,7 +279,7 @@ module "ecs_service" { | |||
| iam_role_description = try(var.service.iam_role_description, null) | |||
| iam_role_permissions_boundary = try(var.service.iam_role_permissions_boundary, null) | |||
| iam_role_tags = try(var.service.iam_role_tags, {}) | |||
| iam_role_statements = lookup(var.service, "iam_role_statements", {}) | |||
| iam_role_statements = lookup(var.service, "iam_role_statements", []) | |||
|
|
|||
| # Task definition | |||
| create_task_definition = try(var.service.create_task_definition, true) | |||
There was a problem hiding this comment.
the container definition input values are now camelCased - so most of these below are incorrect
| @@ -438,26 +437,32 @@ module "ecs_service" { | |||
| security_group_name = try(var.service.security_group_name, null) | |||
| security_group_use_name_prefix = try(var.service.security_group_use_name_prefix, true) | |||
| security_group_description = try(var.service.security_group_description, null) | |||
| security_group_rules = merge( | |||
| security_group_ingress_rules = merge( | |||
There was a problem hiding this comment.
what is the plan for an upgrade guide for these disruptive/breaking changes?
| required_version = ">= 1.0" | ||
| required_version = ">= 1.10" | ||
|
|
||
| required_providers { |
There was a problem hiding this comment.
there are no aws resources in the root module so we don't need the required provider here
Ill get started on these TY, and i think its the ACM module that used version 1.10, so i aligned with that, that said i beleive it was >= 1.10. Ill get this all fixed and answer some questions a little later when Im done with some work stuff |
Co-authored-by: Bryant Biggs <[email protected]>
Co-authored-by: Bryant Biggs <[email protected]>
ah, yes - it does https://github.com/terraform-aws-modules/terraform-aws-acm/blob/155b56d6b4263195ff528a609374de1fab22206b/versions.tf#L2 nvm, leave |
|
Looking forward to this release 💟 |
|
Try this out and i see FYI │ Error: updating ECS Service (arn:aws:ecs:us-east-1:XXXX:service/atlantis/atlantis): operation error ECS: UpdateService, 1 validation error(s) found.
│ - missing required field, UpdateServiceInput.DeploymentConfiguration.Alarms.AlarmNames.
│
│
│ with module.atlantis.module.ecs_service.aws_ecs_service.this[0],
│ on .terraform/modules/atlantis.ecs_service/modules/service/main.tf line 37, in resource "aws_ecs_service" "this":
│ 37: resource "aws_ecs_service" "this" {
│
╵
╷
│ Error: creating IAM Policy (atlantis-20250828015914455200000001): operation error IAM: CreatePolicy, https response error StatusCode: 400, RequestID: dfbf6f42-3209-4d1c-a23d-aa4d24e2eb88, MalformedPolicyDocument: Syntax errors in policy.
│
│ with module.atlantis.module.ecs_service.aws_iam_policy.tasks[0],
│ on .terraform/modules/atlantis.ecs_service/modules/service/main.tf line 1273, in resource "aws_iam_policy" "tasks":
│ 1273: resource "aws_iam_policy" "tasks" {
│
╵
╷
│ Error: updating VPC Security Group Rule (sgr-0b4d9019eb8d93e18)
│
│ with module.atlantis.module.ecs_service.aws_vpc_security_group_egress_rule.this["egress"],
│ on .terraform/modules/atlantis.ecs_service/modules/service/main.tf line 1685, in resource "aws_vpc_security_group_egress_rule" "this":
│ 1685: resource "aws_vpc_security_group_egress_rule" "this" {
│
│ operation error EC2: ModifySecurityGroupRules, https response error StatusCode: 400, RequestID: b4efd66f-05ff-444e-a51e-5af303090d63, api error
│ InvalidParameterValue: You may not specify all protocols and specific ports. Please specify each protocol and port range combination individually, or
│ all protocols and no port range.
|
thanks for this, was this on apply or plan? also I pushed an update |
|
It was on apply indeed. With the latest change i know see |
|
@kwent wtffff ok thank you this is extremely helpful, working on this today |
|
@kwent I believe this occurred from removing try() logic, I may have removed too many per Bryant's feed back, or the wrong ones, ill ping again when i push again |
Sounds good! |
|
@kwent, new push |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR updates the module to align with changes introduced in terraform-aws-modules/ecs/aws v6.x. Specifically, it updates the usage of the ECS service and cluster submodules to reflect current upstream expectations and field structures.
Motivation and Context
Align with upstream module conventions (terraform-aws-modules/ecs/aws v6.x)
Remove deprecated fields and adopt new syntax for ECS cluster and service configuration
Ensure future compatibility and reduce drift from supported patterns
Breaking Changes
Yes.
fargate_capacity_providers has been removed and replaced by default_capacity_provider_strategy, matching upstream behavior.
service_registries must now be set to null or omitted entirely — using an empty list [] will result in a type error. This aligns with upstream, which expects an object, not a list.
Some internal variable defaults and structures were updated to follow upstream expectations, which may cause errors if older formats are used without adjustment.
How Has This Been Tested?
Deployed using the updated module in a live production environment
Verified ECS service behavior and cluster creation
Please let me know if there additional changes or things needed I may of missed